-
Notifications
You must be signed in to change notification settings - Fork 463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding public API test coverage for Aspire.Hosting.Python #5110
Adding public API test coverage for Aspire.Hosting.Python #5110
Conversation
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…ting-python' of github.com:Zombach/aspire into zombach/validate-arguments-of-public-methods#aspire-hosting-python
…methods#aspire-hosting-python
var action = () => new PythonProjectResource(name, executablePath, projectDirectory); | ||
|
||
var exception = Assert.Throws<ArgumentNullException>(action); | ||
Assert.Equal("workingDirectory", exception.ParamName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PythonProjectResource
's parameter is named projectDirectory
which is then passed to the ExecutableResource..ctor
to the parameter named workingDirectory
.
And because the exception is being thrown from ExecutableResource
we get workingDirectory
instead of projectDirectory
. That sounds confusing for the user. Maybe we should have the null check on the .ctor arguments for PythonProjectResource
directly.
@mitchdenny @eerhardt @sebastienros Is there a pattern that we follow in such cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I would add a check, it would be more transparent.
But due to the fact that in PythonProjectResource these arguments are not used at the moment. according to CA1062, we can skip this check, since it is performed in depth.
But if someone adds interaction with arguments to PythonProjectResource, then they will have to add a check for 'null'.
That's why I'm in favor of adding it right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/using-standard-exception-types
✔️ DO set the ParamName property when throwing one of the subclasses of ArgumentException.
This property represents the name of the parameter that caused the exception to be thrown.
I read this as the ParamName should be set to the name of the parameter that was invalid. In this case projectDirectory
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to do the null check in the PythonResource ctor just for clarity. I'm not a fan of the ThrowIfNull method, seems a bit odd to do it that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought it probably doesn't matter to the end result.
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…methods#aspire-hosting-python
Fix PR by feedback
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…methods#aspire-hosting-python
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
…methods#aspire-hosting-python
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…methods#aspire-hosting-python
…methods#aspire-hosting-python
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
…methods#aspire-hosting-python
…methods#aspire-hosting-python
@Zombach thanks once again for all your work on some of these PRs. Appreciate your efforts. |
Initial issues #2649
Issues Check List: Validate arguments of public methods
Microsoft Reviewers: Open in CodeFlow